Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multiline tick labels #83

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

multiline tick labels #83

wants to merge 3 commits into from

Conversation

Fil
Copy link
Member

@Fil Fil commented Dec 11, 2021

@Fil Fil requested a review from mbostock December 11, 2021 16:07
src/axis.js Outdated Show resolved Hide resolved
src/axis.js Outdated Show resolved Hide resolved
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m excited about this. It feels like a big improvement in the expressiveness of axes, with many potentially useful applications.

But we need to think more about the \n? syntax. This introduces a tiny DSL into the tick format. How would you encode a leading question mark in your tick format, if you wanted it, for example? Would it be simpler to have an option that drops repeated values across ticks (on the same line)? And do you need the ability to control this on individual lines? I suspect we could get away with a single boolean option that defaults to true, and no DSL (other than newlines).

@Fil
Copy link
Member Author

Fil commented Dec 11, 2021

How would you encode a leading question mark in your tick format, if you wanted it, for example?

I agree it's better not to introduce a new DSL, but… hmm, why would you ever want that? And you can do it by adding a space before the question mark, or using two question marks if you don't want repeats.

I suspect we could get away with a single boolean option that defaults to true, and no DSL (other than newlines).

Agree, this would probably work well, and it's very simple.

I can see two cases: one is the Month\nYear thing, and the second is someone who returns a "block of text" under each tick, like 

1            2
pie          pies
in           in 
the          the
sky.         sky. 

I don't think someone would ever want to mix the two?

Another alternative with no "\n" DSL would be to allow the tickformat to return an array of lines. But this seems much simpler and "intuitive".

@mbostock
Copy link
Member

I think the \n is fine—that’s not really a DSL (at least not a new one). And in the other case you mentioned, you turn off the repetition filter and just do it yourself. So I think we’re good with just the option to turn off the tick repetition filter).

@Fil
Copy link
Member Author

Fil commented Dec 12, 2021

Should the first line be wrapped in a tspan? (if we're in a multiline context). Might make it easier to manipulate.

We should also, I guess, trim the trailing \s — this way, people who have a hand-crafted tickFormat which returns \n at the end wouldn't suddenly get loads of empty tspans?

@Fil
Copy link
Member Author

Fil commented Dec 13, 2021

To further the discussion:

Added two options:

  • axis.tickMultiline, defaults to "auto"
  • axis.tickNoRepeat, defaults to true

The names of these options is certainly bad. tickNoRepeat does a double negation, so we may want to write as tickSuppressRepeats or something.

One choice tickMultiline offers is between "all in tspans" or "first line in text, next lines in tspan". In my tests having the first line in a tspan shifts it 1px to the right, I don't understand why. I suppose that for simplicity we could always do <text>first line<tspan>second line… and that it would be enough? In that case, tickMultiline is probably not be useful at all (users who might be hit by the change can switch back with an explicit .replace("\n", " ")…), so I guess we can probably remove it?

Last remark: to make empty lines really count when they're followed by a non-empty line (see the axisBottomScaleLinearQuarters example), I used a non-breaking space character—it feels really fragile, there must certainly be a better way.

@Fil
Copy link
Member Author

Fil commented Dec 13, 2021

untitled (3)

@Fil
Copy link
Member Author

Fil commented May 16, 2022

TODO:

  • add a textAnchor option
  • add a dx, dy option

@curran
Copy link
Contributor

curran commented May 16, 2022

Maybe no extra API surface area is required for textAnchor, dx, and dy as they can be set using post-selection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants